feat(widget): show loading indication on retry#7782
Conversation
|
@tenderdeve is attempting to deploy a commit to the cow-dev Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughUpdates the widget iframe retry flow to deduplicate error panels, show a disabled “Loading…” state, recreate the iframe through a reload callback, restore visibility on readiness, and preserve updated listeners across iframe rebuilds. Adds Jest coverage for these behaviors. ChangesIframe retry lifecycle
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant ErrorPanel
participant widgetIframeLoading
participant cowSwapWidget
participant Iframe
participant Widget
User->>ErrorPanel: click Reload
ErrorPanel->>widgetIframeLoading: disable button and show Loading...
widgetIframeLoading->>cowSwapWidget: invoke reload callback
cowSwapWidget->>Iframe: destroy and replace iframe
cowSwapWidget->>Widget: attach iframe and run setup
Widget->>widgetIframeLoading: report ready
widgetIframeLoading->>ErrorPanel: remove error panel
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libs/widget-lib/src/widgetIframeLoading.ts (1)
27-54: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRetry loading state can get stuck forever — timeout isn't re-armed on reload.
The initial
window.setTimeout(..., LOADING_TIMEOUT)(Lines 74-78) is set up once and only cleared inonWidgetReady. The reload click handler (Lines 41-54) never re-arms an equivalent timeout for the retry attempt. If the retried load silently hangs (never fireserror, never becomes ready — exactly the scenarioLOADING_TIMEOUTexists to catch), the button remains disabled with "Loading…" indefinitely with no recovery path. This directly undermines this PR's goal of giving users a visible, resolvable retry indication.🐛 Suggested fix: re-arm the loading timeout on retry
reloadBtn.addEventListener('click', () => { reloadBtn.disabled = true errorText.innerText = 'Loading…' destroy(true) iframe.src = 'about:blank' setTimeout(() => { iframe.src = originalSrc setup() + + timeout = window.setTimeout(() => { + if (cancelled) return + onLoadingError() + }, LOADING_TIMEOUT) }, 100) })Also applies to: 74-78
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@libs/widget-lib/src/widgetIframeLoading.ts` around lines 27 - 54, The retry path in onLoadingError does not re-arm the loading timeout, so a hung reload can leave the panel stuck in the loading state. Update the reload button handler in widgetIframeLoading.ts to start a fresh LOADING_TIMEOUT for each retry attempt, and make sure onWidgetReady still clears whichever timeout is active. Use the existing onLoadingError, onWidgetReady, and reloadBtn flow to keep the retry behavior consistent.
🧹 Nitpick comments (2)
libs/widget-lib/src/widgetIframeLoading.ts (1)
40-47: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNo screen-reader announcement for the loading-state change.
The button/text toggle (
disabled = true,errorText.innerText = 'Loading…') is a purely visual/DOM change; withoutaria-liveonerrorTextoraria-busyon the panel, assistive tech won't announce the retry progress. Consider addingaria-live="polite"toerrorText(or the container) for accessible progress feedback.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@libs/widget-lib/src/widgetIframeLoading.ts` around lines 40 - 47, The reload/loading state update in widgetIframeLoading’s click handler is only visual, so assistive tech won’t announce the retry progress. Update the logic around reloadBtn and errorText to expose the loading transition to screen readers, preferably by adding aria-live="polite" to errorText (or setting aria-busy on the surrounding panel) when switching to “Loading…”. Keep the change localized to the reload button state management in widgetIframeLoading.libs/widget-lib/src/widgetIframeLoading.test.ts (1)
72-91: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing coverage for the retry's actual reload sequence.
Tests verify the disabled/loading UI state and panel dedupe, but none exercise the
setTimeout(..., 100)callback that resetsiframe.srcand callssetup()after a reload click — nor a scenario where the retry itself times out (relevant to the missing timeout re-arm flagged inwidgetIframeLoading.ts). Consider adding a fake-timers test to assertsetup()is invoked andiframe.srcis restored after the retry delay.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@libs/widget-lib/src/widgetIframeLoading.test.ts` around lines 72 - 91, The retry path in widgetIframeLoading lacks coverage for the actual delayed reload flow, including the setTimeout callback that restores iframe.src and re-invokes setup after a retry click. Add a fake-timers test in widgetIframeLoading.test that exercises the retry sequence on the widget iframe loading behavior, then assert the delayed callback runs, setup() is called again, and iframe.src is restored; also cover the retry timeout being re-armed in widgetIframeLoading.ts using the relevant retry handler/setup symbols.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@libs/widget-lib/src/widgetIframeLoading.ts`:
- Around line 27-54: The retry path in onLoadingError does not re-arm the
loading timeout, so a hung reload can leave the panel stuck in the loading
state. Update the reload button handler in widgetIframeLoading.ts to start a
fresh LOADING_TIMEOUT for each retry attempt, and make sure onWidgetReady still
clears whichever timeout is active. Use the existing onLoadingError,
onWidgetReady, and reloadBtn flow to keep the retry behavior consistent.
---
Nitpick comments:
In `@libs/widget-lib/src/widgetIframeLoading.test.ts`:
- Around line 72-91: The retry path in widgetIframeLoading lacks coverage for
the actual delayed reload flow, including the setTimeout callback that restores
iframe.src and re-invokes setup after a retry click. Add a fake-timers test in
widgetIframeLoading.test that exercises the retry sequence on the widget iframe
loading behavior, then assert the delayed callback runs, setup() is called
again, and iframe.src is restored; also cover the retry timeout being re-armed
in widgetIframeLoading.ts using the relevant retry handler/setup symbols.
In `@libs/widget-lib/src/widgetIframeLoading.ts`:
- Around line 40-47: The reload/loading state update in widgetIframeLoading’s
click handler is only visual, so assistive tech won’t announce the retry
progress. Update the logic around reloadBtn and errorText to expose the loading
transition to screen readers, preferably by adding aria-live="polite" to
errorText (or setting aria-busy on the surrounding panel) when switching to
“Loading…”. Keep the change localized to the reload button state management in
widgetIframeLoading.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 15248336-733f-4170-b3cb-175abbd291b6
📒 Files selected for processing (2)
libs/widget-lib/src/widgetIframeLoading.test.tslibs/widget-lib/src/widgetIframeLoading.ts
|
Cloudflare Pages preview mirror Preview branch URL: https://github.com/cowprotocol/cowswap/tree/cf-preview/pr-7782 Source fork branch:
|
elena-zh
left a comment
There was a problem hiding this comment.
Hey @tenderdeve , thank you for the fix.
From a UI perspective, changes look good to me.
But from the functional perspective, it does not work as expected:
- I press on Retry
- nothing is blocking requests to pre-load the widget
- but is cannot be loaded, I still see an error with 'Retry' button
Expected: 'retry' button should load the trading interface if the URL is unblocked. Please compare the behavior with https://dev.widget.cow.fi/
Clicking "Reload" on the widget loading-error panel tore the panel down immediately and showed a blank iframe, so there was no indication the retry was running until the widget finished loading (or the 30s timeout re-fired). Keep the panel mounted and put it into a loading state instead: disable the button (the existing :disabled style already sets cursor: progress) and swap the copy to "Loading…". The panel is cleared by onWidgetReady on success — which now also re-reveals the iframe — or replaced by onLoadingError on another failure. onLoadingError also removes any existing panel first so repeated failures don't stack panels.
Swapping the sandboxed iframe's src in place did not reliably re-fetch the widget, so a failed load never recovered when the URL became reachable again. Retry now tears the frame down and builds a fresh one, which re-emits READY on load and clears the error panel. Expose handler.iframe via a getter so it keeps pointing at the live element after a rebuild. Addresses @elena-zh's review feedback.
19f3f37 to
8b16e69
Compare
|
@elena-zh good catch — the retry only swapped the sandboxed iframe's |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
elena-zh
left a comment
There was a problem hiding this comment.
Thank you, looks good now
fairlighteth
left a comment
There was a problem hiding this comment.
⚠️ AI Review (Codex GPT-5, worked 5m): retry drops runtime-updated widget listeners
Finding: [BLOCKING] Preserve updated event listeners across iframe retry
- Location:
libs/widget-lib/src/cowSwapWidget.ts:148 - This one is important:
setup()always constructsIframeCowEventEmitterwith the initialprops.listeners. updateListeners()updates only the current emitter. AfterreloadIframe()rebuilds the iframe and callssetup(), listeners added through the public API disappear, while old mount-time listeners may return.- The React wrapper uses
updateListeners()when itslistenersprop changes, so this affects the normal integration path.
Suggested fix
- Track the current listeners alongside
providerandcurrentParams; update that value inupdateListeners()and pass it to every new emitter. - Add a widget-level regression test: update listeners, trigger iframe error and Reload, emit an event from the rebuilt
handler.iframe, then verify only the updated listener runs.
Review scope and related context
- The existing reviewer verified that rebuilding the iframe now recovers when the URL becomes available.
- CodeRabbit’s timeout concern is fixed at the current head: retry calls
setup(), which creates a fresh loading context and timeout. - CodeRabbit already raised the accessibility announcement concern, so it is not repeated here.
🤖 Prompt for AI agents
Verify this finding against the current PR head and keep the fix scoped.
Context:
- createCowSwapWidget.setup() initializes IframeCowEventEmitter with the original props.listeners.
- handler.updateListeners() changes only the active emitter.
- reloadIframe() destroys that emitter and calls setup(), restoring the original listeners.
Persist the latest listeners across iframe rebuilds and add a widget-level test covering updateListeners -> loading error -> Reload -> event from the rebuilt iframe.
Generated using the pr-review skill from the CoW Protocol skills repo.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@libs/widget-lib/src/cowSwapWidget.ts`:
- Around line 163-180: The inner zero-argument reloadIframe function shadows the
module-level reloadIframe(iframe, params), causing updateParams to rebuild the
iframe instead of performing the about:blank sandbox transition when
disableWindowOpen changes. Rename or otherwise disambiguate the local helper and
update updateParams to call the intended module-level function; adjust the
affected test to read handler.iframe after updateParams or assert the rebuilt
iframe behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 539b02ac-a22a-4031-b2ae-c58172823049
📒 Files selected for processing (3)
libs/widget-lib/src/cowSwapWidget.tslibs/widget-lib/src/widgetIframeLoading.test.tslibs/widget-lib/src/widgetIframeLoading.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/widget-lib/src/widgetIframeLoading.test.ts
|
Hey @tenderdeve , could you please resolve conflicts and address the comments in this PR? |
setup() rebuilt the event emitter with the initial props.listeners, so a retry after a loading error dropped listeners set via updateListeners() (the React wrapper's normal path) and could resurrect mount-time ones. Track the current listeners and pass them to every new emitter. Adds a regression test.
…retry-loading # Conflicts: # libs/widget-lib/src/cowSwapWidget.test.ts # libs/widget-lib/src/widgetIframeLoading.test.ts
|
@fairlighteth addressed the blocking listener issue: The coderabbit shadowing nit is fixed too (inner helper renamed to |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
libs/widget-lib/src/cowSwapWidget.ts (2)
177-184: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winHide the newly created iframe during the retry loading state.
When
rebuildIframeis invoked, the error panel is still mounted and displaying the "Loading..." state. Because the old iframe was hidden byonLoadingErrorbut the newly created iframe defaults to being visible, appending it immediately makes both the error panel and the blank iframe visible in the container at the same time, causing a layout clash until the widget fully loads.Hide the new iframe immediately;
onWidgetReady()will automatically reveal it once loading succeeds.🐛 Proposed fix
function rebuildIframe(): void { destroy() iframe = createIframe(currentParams) + iframe.style.display = 'none' container.appendChild(iframe) setup() }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@libs/widget-lib/src/cowSwapWidget.ts` around lines 177 - 184, Update rebuildIframe to hide the newly created iframe immediately after createIframe and before appending it to the container. Preserve onWidgetReady’s existing behavior that reveals the iframe after successful loading.
192-197: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winPrevent memory leaks in event listener management.
The
windowListenersarray grows indefinitely becausesetup()andupdateInterceptDeepLinks()continuously push new listeners into it, butdestroy()never empties the array. In a long-lived SPA, updating parameters or triggering iframe rebuilds will accumulate detached closures.Additionally,
interceptDeepLinksListeneris scoped insidesetup()but pushed towindowListenersevery time it updates, compounding the leak on every params update since the old listener remains in the array.To fix both issues, align
interceptDeepLinksListenerwith thewidgetHooksListenerpattern so it can be managed cleanly, and clear thewindowListenersarray at the end ofdestroy().🧹 Proposed refactor to prevent memory leaks
First, move the listener declaration out of
setup()so it isn't recreated on every rebuild (around line 96):let heightChangeListeners: WindowListener[] = [] let widgetHooksListener: WindowListener | null = null + let interceptDeepLinksListener: WindowListener | null = null function setup(): void {Then, stop pushing it into the array during updates (around line 121):
- // 5. Intercept deeplinks navigation in the iframe - let interceptDeepLinksListener: WindowListener | null = null - updateInterceptDeepLinks = () => { if (!iframeWindow) return // ... existing code ... interceptDeepLinksListener = interceptDeepLinks(iframeOrigin, iframeWindow) - windowListeners.push(interceptDeepLinksListener) }Finally, properly clear all listeners in
destroy():// Disconnect all listeners heightChangeListeners.forEach((listener) => window.removeEventListener('message', listener)) windowListeners.forEach((listener) => window.removeEventListener('message', listener)) + windowListeners.length = 0 if (widgetHooksListener) { window.removeEventListener('message', widgetHooksListener) } + if (interceptDeepLinksListener) { + window.removeEventListener('message', interceptDeepLinksListener) + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@libs/widget-lib/src/cowSwapWidget.ts` around lines 192 - 197, Refactor interceptDeepLinksListener to use the same persistent, externally scoped declaration pattern as widgetHooksListener rather than recreating and appending it in setup() or updateInterceptDeepLinks(). Remove its windowListeners registration, manage it directly during updates and destroy(), and clear windowListeners at the end of destroy() after removing its listeners.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@libs/widget-lib/src/cowSwapWidget.ts`:
- Around line 177-184: Update rebuildIframe to hide the newly created iframe
immediately after createIframe and before appending it to the container.
Preserve onWidgetReady’s existing behavior that reveals the iframe after
successful loading.
- Around line 192-197: Refactor interceptDeepLinksListener to use the same
persistent, externally scoped declaration pattern as widgetHooksListener rather
than recreating and appending it in setup() or updateInterceptDeepLinks().
Remove its windowListeners registration, manage it directly during updates and
destroy(), and clear windowListeners at the end of destroy() after removing its
listeners.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 3143c5af-b33a-4a6f-9fab-c670dc044017
📒 Files selected for processing (2)
libs/widget-lib/src/cowSwapWidget.test.tslibs/widget-lib/src/cowSwapWidget.ts
Fixes #7734
Clicking Reload on the widget's loading-error panel removed the panel right away and showed a blank iframe, so nothing indicated the retry was in progress until the widget loaded (or the 30s timeout re-fired).
Now the panel stays mounted in a loading state: the button is disabled (the
:disabledrule already shipscursor: progress) and the text switches to "Loading…". It's cleared byonWidgetReadyon success — which now also re-reveals the iframe — or replaced byonLoadingErroron another failure.onLoadingErroralso clears any existing panel first so repeated failures don't stack.Tests added to
widgetIframeLoading.test.ts: reload shows the loading state, ready removes the panel and reveals the iframe, and repeated failures keep a single panel.Summary by CodeRabbit